Conversation
There was a problem hiding this comment.
Pull request overview
Integrates the new ai_resources_page MkDocs plugin and updates AI Resources page behavior/config so the page can use plugin-generated paths and richer category metadata.
Changes:
- Enabled the
ai_resources_pageplugin inmkdocs.yml. - Refactored AI Resources page client-side JS to consume plugin-generated
data-pathURLs for copy/download/view actions. - Added per-category descriptions under
categories_infoinllms_config.json.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| mkdocs.yml | Enables the ai_resources_page plugin in the MkDocs build. |
| material-overrides/main.html | Updates AI Resources interactivity to use plugin-provided URLs and adds explanatory comments. |
| llms_config.json | Adds category description metadata for AI/LLM context and UI clarity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| a.addEventListener('click', (e) => { | ||
| const url = toAbsolute(a.getAttribute('data-path') || ''); | ||
| if (!url) return; | ||
| const path = a.getAttribute('data-path'); | ||
| if (!path) return; | ||
| if (a.getAttribute('target') !== '_blank') { | ||
| e.preventDefault(); | ||
| window.open(url, '_blank', 'noopener'); | ||
| window.open(path, '_blank', 'noopener'); | ||
| } |
There was a problem hiding this comment.
The .llms-view click handler is effectively dead code because the script always sets target="_blank" when data-path is present, so the if (a.getAttribute('target') !== '_blank') branch will never run. Consider removing this listener or adjusting the logic so it covers the intended scenario.
| const downloadViaFetch = async (url, filename) => { | ||
| try { | ||
| const res = await fetch(url, { credentials: 'omit' }); |
There was a problem hiding this comment.
In downloadViaFetch, the object URL is revoked immediately after triggering link.click(). In some browsers the download may not have started yet, which can lead to flaky/failed downloads. Revoke the object URL after a short timeout (or other deferred cleanup) instead of immediately.
| a.setAttribute('href', url); | ||
| a.setAttribute('target', '_blank'); | ||
| a.setAttribute('rel', 'noopener'); |
There was a problem hiding this comment.
New tab links elsewhere in this template use rel="noopener noreferrer", but this code sets only noopener. For consistency and to avoid leaking the referrer to the raw file URL, consider using noopener noreferrer here as well (and include noreferrer in the window.open feature string if keeping the JS fallback).
| exclude: | ||
| - ai/* | ||
| - awesome-nav | ||
| - ai_resources_page |
There was a problem hiding this comment.
Enabling the ai_resources_page MkDocs plugin will break builds unless the plugin package is installed in the build environment. Please ensure the dependency is added/pinned in the Python requirements used by CI/local builds (or otherwise vendored) so mkdocs can import it.
| - ai_resources_page |
There was a problem hiding this comment.
Overall, I think we need to change the approach we're taking with this script.
Rather than wiring behavior to specific class names (e.g. “find this class and do X”), it would be cleaner to hook into a single wrapper that contains all actions and iterate over them. From there, behavior can be driven by the action type—links, downloads, clipboard, etc.
For example, standard links shouldn’t require any additional JavaScript at all; they should rely on the browser’s default behavior. JavaScript should only be involved for action types that actually need it.
Downloads are a good example of this. I didn’t initially expect we’d be handling downloads via blobs, so this may require some adjustments to the schema, since downloads aren't really "links" when using the blob approach.
Clipboard actions would continue to be handled client-side as they are now.
This is just the first PR I've taken a look at and I haven't spun anything up yet. I also haven't looked at the plugin logic yet, but these are some initial thoughts.
| "Networks": { | ||
| "description": "Information about Polkadot networks." | ||
| }, | ||
| "Polkadot Protocol": { |
There was a problem hiding this comment.
I'm not so sure about using the display-like labels as keys. I think this could potentially cause issues down the road depending on how things evolve. It might make more sense to have like a "label" key inside each category with the display name and use lower case/underscores when necessary.
Not sure how it's used so as I dig into this a bit more, I might have more comments on this.
| a.setAttribute('rel', 'noopener'); | ||
| } | ||
|
|
||
| a.addEventListener('click', (e) => { |
There was a problem hiding this comment.
I don't know how this previously made it's way in. I feel like I made a bunch of comments on those PRs about adding an event listener to an a tag. Duplicating the browsers default functionality.
Not that it really matters, because there shouldn't be any extra logic needed for the View functionality. None of this is necessary anymore.
| }); | ||
|
|
||
| // VIEW buttons: open raw files in a new tab | ||
| document.querySelectorAll('.llms-view').forEach((a) => { |
There was a problem hiding this comment.
This should just be a link. We shouldn't require any logic at all on this side of things for this to work.
| if (abs) a.setAttribute('href', abs); | ||
| // Plugin generates the correct absolute path in data-path | ||
| const url = a.getAttribute('data-path'); | ||
| if (url) a.setAttribute('href', url); |
There was a problem hiding this comment.
we shouldn't be setting an href on this side. That should be done plugin side if needed.
| path.split('/').pop() || | ||
| 'download.txt'; | ||
| downloadViaFetch(url, filename); | ||
| const path = a.getAttribute('data-path'); |
There was a problem hiding this comment.
you're already getting this value on line 417
| exclude: | ||
| - ai/* | ||
| - awesome-nav | ||
| - ai_resources_page |
There was a problem hiding this comment.
I don't know if we talked about this, but have you looked into accepting a file path in the config here for where the AI resources page should live, and then on the plugin side, building it from scratch so that we don't need an empty placeholder file in the docs dir? There could be lots of side effects from this approach. But just curious
2 participants
Requires these other PRs to work:
mkdocs-plugins: papermoonio/mkdocs-plugins#8
polkadot-docs: polkadot-developers/polkadot-docs#1485
This pull request introduces improvements to the AI Resources page interactivity and enhances category metadata for the Polkadot documentation. The main changes include adding detailed descriptions to each knowledge category in
llms_config.json, and refactoring the client-side JavaScript inmaterial-overrides/main.htmlto rely on plugin-generated absolute paths for resource handling. Additionally, the newai_resources_pageplugin is enabled inmkdocs.ymlfor generating the AI Resources page.Enhancements to category metadata:
categories_infosection tollms_config.json, providing descriptions for each documentation category to improve context and clarity for users.AI Resources page interactivity and plugin integration:
ai_resources_pageplugin inmkdocs.ymlto generate a dedicated AI Resources page.material-overrides/main.htmlto use plugin-generated absolute paths for download, copy, and view actions, simplifying path handling and improving reliability.